Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media Multi-Reference Feature #1241

Conversation

rogernelson
Copy link
Collaborator

Fixes #630

Summarize your change.

This branch is an initial prototype for the changes and discussion initially proposed in this working document.

To summarize, there are often uses cases when a single conceptual clip can have multiple different media representations. A common example is high-resolution and proxy-resolution representations of the same media where the high-resolution is used when users have access to the file path and hardware appropriate for playback and proxy-resolution for when they do not. In these situations it's convenient for the Clip to have a reference to each media representation so we can easily toggle between them based on our needs.

To accommodate these workflows, this branch transforms the single media reference contained within a clip into a map/dictionary of media references. Below is what the difference looks like when serialized.

before:

"media_reference" : {
    "OTIO_SCHEMA" : "MissingReference.1",
    "available_range" : null,
    "available_image_bounds" : null,
    "metadata" : {},
    "name" : ""
}

after:

"active_media_reference": "DEFAULT_MEDIA",
"media_references" : {
    "DEFAULT_MEDIA": {
        "OTIO_SCHEMA" : "MissingReference.1",
        "available_range" : null,
        "available_image_bounds" : null,
        "metadata" : {},
        "name" : ""
    }
}

As can be seen above, the later format is extensible allowing new media references to be added to the media_references map. The key set in the active_media_reference will be the media reference returned using the existing media_reference properties. To change the active reference, the user can simply just pass the name of the key of the new active reference to the new active_media_reference property.

Reference associated tests.

test_clip.{py,cpp} were modified to test the new functionality. All other existing tests access that media_reference (test_clip, test_track, test_stack, etc) ensure the existing functionality continues to work.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
@meshula meshula added TSC Slated for discussion at the next TSC meeting enhancement A request for something new. labels Mar 15, 2022
Copy link
Collaborator

@reinecke reinecke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, the ergonomics look like they'd be pretty good on this!

I just had a few comments for discussion about having a standard set of reference keys and bike shedding the name of `active_media_reference.

src/opentimelineio/clip.h Outdated Show resolved Hide resolved
@meshula
Copy link
Collaborator

meshula commented Mar 16, 2022

@rogernelson This proposal has iterated greatly in a good direction, since the original document

https://docs.google.com/document/d/13jrl2qtWBTSFqMTVQh28LWc16uYvQ79w-7TfX92JaIM/edit?usp=sharing

and a great many decisions have been taken since then, making it hard to parse the implementation versus the design intention.

As I read through the comments and changes, I keep thinking I wish I could refer the current state against a technical design document. Could the current state of the design and its behaviors be added to the existing document? I think a paragraph summary of the addition to the schema, and the current state of the design would suffice.

I added a Current Status block at the document, pointing to this PR, as a place where I thought it could go.

@rogernelson
Copy link
Collaborator Author

I added a Current Status block at the document, pointing to this PR, as a place where I thought it could go.

Done. I listed the API additions as well as describing their intended usage. I also tried to summarize the various points of discussion that have come up here. Please let me know if there's anything else you think should be in there.

@rogernelson
Copy link
Collaborator Author

rogernelson commented Mar 18, 2022

The python 2.7 build is failing because of this:

>- - *media_references*: media_references(self: opentimelineio._otio.Clip) -> Dict[str, opentimelineio._otio.MediaReference]
?                                                                                 ^^^
+ - *media_references*: media_references(self: opentimelineio._otio.Clip) -> Dict[unicode, opentimelineio._otio.MediaReference]
?       

The difference is str vs. unicode

I build the python bindings with python3, so that probably explains why 2.7 is failing. But I am curious, why is it putting in the function signature for media_references? It doesn't seem to do that for anything else...

@codecov-commenter
Copy link

Codecov Report

Merging #1241 (ed0964b) into main (aa5351c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1241      +/-   ##
==========================================
+ Coverage   86.15%   86.18%   +0.03%     
==========================================
  Files         196      196              
  Lines       19632    19716      +84     
  Branches     2302     2302              
==========================================
+ Hits        16913    16992      +79     
- Misses       2161     2166       +5     
  Partials      558      558              
Flag Coverage Δ
py-unittests 86.18% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/opentimelineio/errorStatus.h 100.00% <ø> (ø)
src/opentimelineio/clip.cpp 85.07% <100.00%> (+12.85%) ⬆️
src/opentimelineio/errorStatus.cpp 42.85% <100.00%> (+2.11%) ⬆️
src/opentimelineio/typeRegistry.cpp 82.35% <100.00%> (+1.61%) ⬆️
...pentimelineio-bindings/otio_errorStatusHandler.cpp 66.07% <100.00%> (+1.25%) ⬆️
...entimelineio-bindings/otio_serializableObjects.cpp 91.78% <100.00%> (+0.17%) ⬆️
...timelineio/console/autogen_serialized_datamodel.py 76.98% <100.00%> (ø)
tests/test_clip.py 97.56% <100.00%> (+0.95%) ⬆️
src/opentimelineio/serializableObject.h 86.53% <0.00%> (-2.83%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5351c...ed0964b. Read the comment docs.

Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I left some questions about corner cases.

src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
src/opentimelineio/clip.cpp Show resolved Hide resolved
Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the last minute touch ups! lgtm

@jminor jminor changed the title Multimedia prototype Media Multi-Reference Feature Apr 13, 2022
@jminor jminor merged commit 9f55adf into AcademySoftwareFoundation:main Apr 13, 2022
@jminor
Copy link
Collaborator

jminor commented Apr 13, 2022

Thanks for the contribution @rogernelson !!!

@rogernelson rogernelson deleted the adsk/nelsonr/multimedia_proto branch April 13, 2022 20:17
jminor pushed a commit that referenced this pull request May 2, 2022
* Upgraded the Clip API and schema to support multiple media references in a single Clip.
* New API is introduced to allow for setting a dictionary of media references indexed by a string key, and API for choosing which of those is active. The old API for getting/setting a single media reference is retained for backwards compatibility, and for use cases where only a single media reference is needed.
* Note that the Clip schema version is updated to Clip.2, which is not supported by older versions of OTIO.
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for something new. TSC Slated for discussion at the next TSC meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Multiple Media Reference on clip
8 participants